refactor: share Pages ISR cache runtime#684
refactor: share Pages ISR cache runtime#684james-elicx merged 1 commit intocodex/pages-api-route-runtimefrom
Conversation
|
/bigbonk |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean refactor that does exactly what the AGENTS.md guidelines prescribe: move real behavior out of the generated entry into a typed, testable module, keeping the codegen layer as thin delegation glue.
Verified:
-
getRequestExecutionContextidentity -- The generated entry imports fromshims/request-context.js(aliased as_getRequestExecutionContext), andserver/isr-cache.tsimports the same function from../shims/request-context.js. Both resolve to the same module. The shared module'sgetRequestExecutionContext()?.waitUntil(promise)is equivalent to the old inlineif (ctx && typeof ctx.waitUntil === "function") ctx.waitUntil(promise)pattern. -
tagsdefault handling -- The old inlineisrSetusedtags || []. The sharedisrSetusestags ?? []. Both produce[]forundefined/nullinputs. No behavioral regression. -
isrCacheKeybuildId injection -- The thin wrapper correctly passesbuildId || undefined, matching the old inline behavior wherebuildIdwas a top-level const that could benull. The|| undefinedcoercion ensures the shared function's optional parameter semantics work correctly. -
pendingRegenerationsscope -- The old inline code used a module-scopednew Map(). The shared module usesSymbol.for()onglobalThisfor cross-environment dedup. This is strictly better for the Pages Router case since it means the dedup map is shared if the module is loaded in multiple Vite environments. -
Tests -- Assertions correctly shifted from checking inline implementation details (regex for
renderFn()ordering,waitUntilplacement,fnv1a64) to checking delegation patterns (import presence, wrapper function shapes, absence of removed code).
One observation (not blocking): the App Router entry still inlines its own ISR helpers with a comment explaining why (RSC environment module-evaluation constraints). A follow-up to share those too would further reduce duplication, but the comment documents the constraint clearly.
|
Review posted and PR approved. Here's a summary of what I verified: PR #684 swaps the Pages Router server entry from an inline ISR implementation to delegating to the shared Key verification points:
|
* refactor: extract pages api route runtime * refactor: share Pages ISR cache runtime (#684) * fix: address Pages API route review feedback - Replace fragile error.message string matches with typed PagesBodyParseError (statusCode: 413) thrown from pages-node-compat.ts and api-handler.ts; the existing instanceof catch in handlePagesApiRoute handles both 400 and 413 - Fix res.setHeader('set-cookie') to replace instead of append on repeated calls, matching Node.js ServerResponse semantics - Extract shared getMediaType/isJsonMediaType helpers and the body-parse error class into pages-media-type.ts to eliminate drift between api-handler.ts and pages-node-compat.ts - Add unit tests for 404 (null match), missing default export (500), redirect (1-arg and 2-arg), writeHead header lowercasing/array joining, setHeader/ getHeader round-trip, and set-cookie replace-not-append parity --------- Co-authored-by: James <james@eli.cx>
2 participants
Summary
server/isr-cache.tsruntime instead of embedding a second ISR implementation in the generated templateisrGet,isrSet,triggerBackgroundRegeneration, andisrCacheKeythrough typed helpersVerification
vp check packages/vinext/src/entries/pages-server-entry.ts tests/entry-templates.test.tsvp test run tests/entry-templates.test.ts tests/isr-cache.test.ts -uvp test run tests/pages-router.test.ts -t "renders pages with getStaticPaths + getStaticProps|returns 404 for paths not in getStaticPaths when fallback is false|keeps ISR cache-fill rerenders isolated from the streamed render state|wraps stale regeneration in a fresh unified request context"vp run vinext#buildWritten by Codex.